Implement GH-13514 PASSWORD_ARGON2 from OpenSSL 3.2 #13635

remicollet · 2024-03-08T11:54:48Z

=====================================================================
PHP         : /work/build/phpmaster/sapi/cli/php 
PHP_SAPI    : cli
PHP_VERSION : 8.4.0-dev
ZEND_VERSION: 4.4.0-dev
PHP_OS      : Linux - Linux builder.remirepo.net 6.7.7-200.fc39.x86_64 #1 SMP PREEMPT_DYNAMIC Fri Mar  1 16:53:59 UTC 2024 x86_64
INI actual  : /work/build/phpmaster/tmp-php.ini
More .INIs  :  
---------------------------------------------------------------------
PHP         : /work/build/phpmaster/sapi/cgi/php-cgi 
PHP_SAPI    : cgi-fcgi
PHP_VERSION : 8.4.0-dev
ZEND_VERSION: 4.4.0-dev
PHP_OS      : Linux - Linux builder.remirepo.net 6.7.7-200.fc39.x86_64 #1 SMP PREEMPT_DYNAMIC Fri Mar  1 16:53:59 UTC 2024 x86_64
INI actual  : /work/build/phpmaster/tmp-php.ini
More .INIs  : 
--------------------------------------------------------------------- 
---------------------------------------------------------------------
PHP         : /work/build/phpmaster/sapi/phpdbg/phpdbg 
PHP_SAPI    : phpdbg
PHP_VERSION : 8.4.0-dev
ZEND_VERSION: 4.4.0-dev
PHP_OS      : Linux - Linux builder.remirepo.net 6.7.7-200.fc39.x86_64 #1 SMP PREEMPT_DYNAMIC Fri Mar  1 16:53:59 UTC 2024 x86_64
INI actual  : /work/build/phpmaster/tmp-php.ini
More .INIs  : 
---------------------------------------------------------------------
CWD         : /work/build/phpmaster
Extra dirs  : 
VALGRIND    : Not used
=====================================================================
Running selected tests.
Spawning 8 workers... Done in 0.02s
=====================================================================

PASS Test that password_hash() does not overread buffers when a short hash is passed [ext/standard/tests/password/password_bcrypt_short.phpt] 
PASS Test that the password parameter is marked sensitive. [ext/standard/tests/password/password_hash_sensitive_parameter.phpt] 
PASS Test that the value of PASSWORD_DEFAULT matches PASSWORD_BCRYPT [ext/standard/tests/password/password_default.phpt] 
PASS Test normal operation of password_get_info() [ext/standard/tests/password/password_get_info.phpt] 
PASS Test normal operation of password_needs_rehash() [ext/standard/tests/password/password_needs_rehash.phpt] 
PASS Test normal operation of password_get_info() with Argon2i and Argon2id [ext/standard/tests/password/password_get_info_argon2.phpt] 
PASS Test removed support for explicit salt option [ext/standard/tests/password/password_removed_salt_option.phpt] 
PASS Test normal operation of password_verify) [ext/standard/tests/password/password_verify.phpt] 
PASS Test normal operation of password_needs_rehash() with Argon2i and Argon2id [ext/standard/tests/password/password_needs_rehash_argon2.phpt] 
PASS Test error operation of password_needs_rehash() [ext/standard/tests/password/password_needs_rehash_error.phpt] 
PASS Test normal operation of password_verify() with Argon2i and Argon2id [ext/standard/tests/password/password_verify_argon2.phpt] 
PASS Test error operation of password_verify() [ext/standard/tests/password/password_verify_error.phpt] 
PASS Test normal operation of password_hash() [ext/standard/tests/password/password_hash.phpt] 
PASS Basic features of password_hash [ext/openssl/tests/openssl_password.phpt] 
PASS Compatibility of password_hash [ext/openssl/tests/openssl_password_compat.phpt] 
PASS Bug #75221 (Argon2i always throws NUL at the end) [ext/standard/tests/password/bug75221.phpt] 
PASS Test error operation of password_hash() with bcrypt hashing [ext/standard/tests/password/password_bcrypt_errors.phpt] 
PASS Test normal operation of password_hash() with Argon2i and Argon2id [ext/standard/tests/password/password_hash_argon2.phpt] 
PASS Test error operation of password_hash() [ext/standard/tests/password/password_hash_error.phpt] 
SKIP Test error operation of password_hash() with Argon2i and Argon2id [ext/standard/tests/password/password_hash_error_argon2.phpt] reason: argon2 not provided by standard
=====================================================================
Number of tests :    20                19
Tests skipped   :     1 (  5.0%) --------
Tests warned    :     0 (  0.0%) (  0.0%)
Tests failed    :     0 (  0.0%) (  0.0%)
Tests passed    :    19 ( 95.0%) (100.0%)
---------------------------------------------------------------------
Time taken      : 2.367 seconds
=====================================================================

remicollet · 2024-03-08T12:56:06Z

Quick bench

Sodium: 0.565

Argon2 with threads=1:  1.012"
Argon2 with threads=2:  0.816"
Argon2 with threads=4:  0.436"
Argon2 with threads=6:  0.314"
Argon2 with threads=8:  0.257"
Argon2 with threads=10: 0.221"
Argon2 with threads=12: 0.197"

OpenSSL with threads=1:  0.997"
OpenSSL with threads=2:  1.036"
OpenSSL with threads=4:  0.619"
OpenSSL with threads=6:  0.482"
OpenSSL with threads=8:  0.432"
OpenSSL with threads=10: 0.377"
OpenSSL with threads=12: 0.343"

remicollet · 2024-03-08T15:23:58Z

Notice: openssl_password_hash and openssl_password_verify are probably not very useful, but very small and allow to compare/check results from other implementation.

As openssl is (usually) loaded before sodium, it will be used by default when libargon2 is not used.

remicollet · 2024-03-13T10:00:58Z

I'm not sure about the second commit (simplify init/shutdown)

with it, we are close to what ext/sodium do and init/shutdown is simpler
without, we have a better separation of password-related code

remicollet · 2024-03-13T14:56:11Z

Additional notice:

As we need base64 without padding, a later feature/PR (ext/standard) will be to add a NOPADDING option to php_base64_encode to avoid adding/removing the '=' char.

P.S. see PR #13698

remicollet · 2024-03-26T12:59:05Z

Rebased and switch to php_base64_encode_ex (no padding)

bukka

Think only issue might be usage OSSL_set_max_threads that needs more checking and possibly changing. Other than that, there are mostly minor things to change.

ext/openssl/openssl_pwhash.c

remicollet · 2024-04-02T06:50:46Z

@bukka requested changes done

remicollet · 2024-05-22T09:40:46Z

Notice: the threads does not affect the hash result, only lanes do.

Using 1 thread per lane allows faster hashing.

bukka · 2024-05-27T12:19:47Z

Using 1 thread per lane allows faster hashing.

Yeah and from your results it is visible that only one thread means twice slow down compare to sodium in hashing so might not be the best idea prefer it over sodium...

I was a bit busy but should have time in June to look into it properly (hopefully in the next couple of weeks - I know I said this almost 2 months ago but this time I give a bit more priority).

remicollet · 2024-07-15T13:40:49Z

ping @bukka

Sorry can you please review, I'd really like to see this land in 8.4, before beta

P.S. Branch as already diverged, more time will give more work to merge

bukka · 2024-07-15T14:03:45Z

@remicollet See #14734 . It's quite a lot of work and we will also need to to introduce some way to better configure providers for that context so I'm afraid it won't be ready for 8.4 as I'm currently primarily focusing on other projects and this is more a side thing.

Without that this change is not safe for ZTS so the only way forward for this in 8.4 is to allow it only for non ZTS builds. Using a single thread does not make much sense to me as it would decrease performance.

remicollet · 2024-07-19T07:59:42Z

@remicollet See #14734 . It's quite a lot of work and we will also need to to introduce some way to better configure providers for that context so I'm afraid it won't be ready for 8.4 as I'm currently primarily focusing on other projects and this is more a side thing.

Without that this change is not safe for ZTS so the only way forward for this in 8.4 is to allow it only for non ZTS builds. Using a single thread does not make much sense to me as it would decrease performance.

PR rebased

Also add a --with-openssl-argon2 build option, so builder will have choice to keep using other algo (ex sodium)
This includes check for ZTS and for 3.2

Check for ZTS could be removed when #14734 will be merged

remicollet · 2024-08-02T13:12:34Z

@bukka sorry to ping you again, this is ready for final review/merge

bukka

@remicollet sorry for the delay - I actually started review 2 weeks ago but didn't finish it. Looks good except that libctx that shouldn't be there if it's just for non ZTS. Ohter comments mostly NITs and CS

ext/openssl/config0.m4

ext/openssl/openssl_pwhash.c

ext/openssl/tests/openssl_password.phpt

remicollet · 2024-08-19T11:58:27Z

Rebased,

All requested changes done, ready for another review

bukka

Looks good to me, just one thing and one NIT.

ext/openssl/openssl_pwhash.c

remicollet · 2024-08-25T06:49:25Z

Looks good to me, just one thing and one NIT.

All done

remicollet · 2024-09-02T09:18:00Z

@bukka can we please move forward, this PR is 6 months old now and all nits fixed

bukka

One hopefully last thing

ext/openssl/openssl_pwhash.c

remicollet · 2024-09-02T10:09:46Z

One hopefully last thing

Done

remicollet · 2024-09-02T11:14:47Z

Thanks,

Squashed and merged as 32c5ce3 and b81f972

bukka · 2024-09-02T11:35:35Z

@remicollet you should get blessing of RM before merging this though. See https://github.com/php/policies/blob/main/release-process.rst#beta-releases .

remicollet requested review from kocsismate and bukka as code owners March 8, 2024 11:54

github-actions bot added Category: Build System Extension: openssl Extension: standard labels Mar 8, 2024

remicollet mentioned this pull request Mar 8, 2024

[RFE] Implements PASSWORD_ARGON2 from OpenSSL 3.2 #13514

Closed

remicollet mentioned this pull request Mar 8, 2024

Argon2 hashing using OpenSSL #12701

Closed

remicollet added the Feature label Mar 8, 2024

remicollet mentioned this pull request Mar 13, 2024

add padding option to base64_encode #13698

Closed

remicollet force-pushed the issue-13514-openssl-password branch from 2c89110 to 756ae17 Compare March 26, 2024 12:58

remicollet force-pushed the issue-13514-openssl-password branch from 756ae17 to cfed1f7 Compare March 26, 2024 12:59

bukka requested changes Mar 30, 2024

View reviewed changes

remicollet requested a review from bukka April 2, 2024 09:12

remicollet force-pushed the issue-13514-openssl-password branch 2 times, most recently from f5ddf51 to 726600d Compare April 2, 2024 12:25

bukka mentioned this pull request Jun 30, 2024

OpenSSL: Use custom libctx for each thread in ZTS #14734

Open

remicollet force-pushed the issue-13514-openssl-password branch from 726600d to f22d8a3 Compare July 19, 2024 07:56

bukka requested changes Aug 6, 2024

View reviewed changes

remicollet added 7 commits August 19, 2024 12:22

Implement phpGH-13514 PASSWORD_ARGON2 from OpenSSL 3.2

3c04dfc

simplify init/shutdown

7893c82

use php_base64_encode_ex

2ac0b04

- rename macros - use openssl RAND_bytes - CS

0460a1e

add --with-openssl-argon2 build option

668b3f0

check OSSL_KDF_PARAM_ARGON2_LANES instead of OSSL_set_max_threads

3b67b4e

Cleanup and CS

ddcc40f

remicollet force-pushed the issue-13514-openssl-password branch from f22d8a3 to ddcc40f Compare August 19, 2024 11:57

remicollet requested a review from bukka August 19, 2024 11:57

bukka requested changes Aug 24, 2024

View reviewed changes

ext/openssl/openssl_pwhash.c Outdated Show resolved Hide resolved

ext/openssl/openssl_pwhash.c Outdated Show resolved Hide resolved

save/restore old threads config + CS

74880be

remicollet requested a review from bukka August 25, 2024 06:49

bukka requested changes Sep 2, 2024

View reviewed changes

ext/openssl/openssl_pwhash.c Outdated Show resolved Hide resolved

ext/openssl/openssl_pwhash.c Outdated Show resolved Hide resolved

remove unneeded check

0dd9cc7

remicollet requested a review from bukka September 2, 2024 10:09

bukka approved these changes Sep 2, 2024

View reviewed changes

remicollet merged commit 32c5ce3 into php:master Sep 2, 2024
9 of 10 checks passed

remicollet deleted the issue-13514-openssl-password branch September 2, 2024 11:01

cmb69 mentioned this pull request Sep 2, 2024

Support --with-openssl-argon2 on Windows #15713

Merged

jrfnl mentioned this pull request Oct 5, 2024

PHP 8.4 | NewFunctions: detect more new functions PHPCompatibility/PHPCompatibility#1783

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement GH-13514 PASSWORD_ARGON2 from OpenSSL 3.2 #13635

Implement GH-13514 PASSWORD_ARGON2 from OpenSSL 3.2 #13635

remicollet commented Mar 8, 2024 •

edited

Loading

remicollet commented Mar 8, 2024 •

edited

Loading

remicollet commented Mar 8, 2024

remicollet commented Mar 13, 2024 •

edited

Loading

remicollet commented Mar 13, 2024 •

edited

Loading

remicollet commented Mar 26, 2024

bukka left a comment

remicollet commented Apr 2, 2024 •

edited

Loading

remicollet commented May 22, 2024

bukka commented May 27, 2024

remicollet commented Jul 15, 2024 •

edited

Loading

bukka commented Jul 15, 2024

remicollet commented Jul 19, 2024

remicollet commented Aug 2, 2024

bukka left a comment •

edited

Loading

remicollet commented Aug 19, 2024 •

edited

Loading

bukka left a comment

remicollet commented Aug 25, 2024

remicollet commented Sep 2, 2024

bukka left a comment

remicollet commented Sep 2, 2024

remicollet commented Sep 2, 2024

bukka commented Sep 2, 2024

Implement GH-13514 PASSWORD_ARGON2 from OpenSSL 3.2 #13635

Implement GH-13514 PASSWORD_ARGON2 from OpenSSL 3.2 #13635

Conversation

remicollet commented Mar 8, 2024 • edited Loading

remicollet commented Mar 8, 2024 • edited Loading

remicollet commented Mar 8, 2024

remicollet commented Mar 13, 2024 • edited Loading

remicollet commented Mar 13, 2024 • edited Loading

remicollet commented Mar 26, 2024

bukka left a comment

Choose a reason for hiding this comment

remicollet commented Apr 2, 2024 • edited Loading

remicollet commented May 22, 2024

bukka commented May 27, 2024

remicollet commented Jul 15, 2024 • edited Loading

bukka commented Jul 15, 2024

remicollet commented Jul 19, 2024

remicollet commented Aug 2, 2024

bukka left a comment • edited Loading

Choose a reason for hiding this comment

remicollet commented Aug 19, 2024 • edited Loading

bukka left a comment

Choose a reason for hiding this comment

remicollet commented Aug 25, 2024

remicollet commented Sep 2, 2024

bukka left a comment

Choose a reason for hiding this comment

remicollet commented Sep 2, 2024

remicollet commented Sep 2, 2024

bukka commented Sep 2, 2024

remicollet commented Mar 8, 2024 •

edited

Loading

remicollet commented Mar 8, 2024 •

edited

Loading

remicollet commented Mar 13, 2024 •

edited

Loading

remicollet commented Mar 13, 2024 •

edited

Loading

remicollet commented Apr 2, 2024 •

edited

Loading

remicollet commented Jul 15, 2024 •

edited

Loading

bukka left a comment •

edited

Loading

remicollet commented Aug 19, 2024 •

edited

Loading